Skip to content

Add RealtimeSyncOff and refactor interface of SyncMode#772

Merged
hackerwins merged 12 commits intomainfrom
sync-off-mode
Apr 11, 2024
Merged

Add RealtimeSyncOff and refactor interface of SyncMode#772
hackerwins merged 12 commits intomainfrom
sync-off-mode

Conversation

@chacha912
Copy link
Contributor

@chacha912 chacha912 commented Apr 9, 2024 •
edited
Loading

What this PR does / why we need it?

The SyncOff mode will allow users to maintain the watch stream functionality while disabling synchronization.

Does this PR introduce a user-facing change?:

- 1️⃣ With the addition of the `RealtimeSyncOff` mode, the `SyncMode` has been simplified into four options. Usage is as follows:

// as-is
await client.pause(doc); // manual sync
await client.resume(doc); // realtime sync(pushpull)
client.pauseRemoteChanges(doc); // pushonly 
client.resumeRemoteChanges(doc); // pushpull 

// to-be
await client.changeSyncMode(doc, SyncMode.Manual);
await client.changeSyncMode(doc, SyncMode.Realtime); // Same as PushPull mode
await client.changeSyncMode(doc, SyncMode.RealtimePushOnly); 
await client.changeSyncMode(doc, SyncMode.RealtimeSyncOff); // Added RealtimeSyncOff mode 

- 2️⃣ In manual sync mode, the `sync()` function can now only be executed in pushpull mode.

await client.sync(doc, SyncMode.PushOnly) // Deprecated: Only pushpull synchronization is supported now.

- 3️⃣ The option to specify manual sync during attachment has been updated.

// as-is 
await client.attach(doc, { isRealtimeSync: false });

// to-be
await client.attach(doc, { syncMode: SyncMode.Manual });

Any background context you want to provide?

  • I'd like to discuss the user interface
  1. With the addition of SyncOff mode, the existing functions (pauseRemoteChanges, resumeRemoteChanges) have been unified under changeRealtimeSyncMode.
// as-is
client.pauseRemoteChanges(doc); // pushonly 
client.resumeRemoteChanges(doc); // pushpull 

// to-be
client.changeRealtimeSyncMode(doc, SyncMode.PushPull); // pushpull
client.changeRealtimeSyncMode(doc, SyncMode.PushOnly); // pushonly
client.changeRealtimeSyncMode(doc, SyncMode.SyncOff); // syncoff
  1. The changeRealtimeSyncMode is currently designated for use only in realtime sync. Therefore, if you want to resume from manual mode to pushonly mode, you need to call resume() and then change the sync mode. By default, resume() operates in push-pull mode, but would it be necessary to provide an option to set the sync mode during the resume process?
// manual -> realtime(pushonly)
// as-is
await client.pause(doc); // manual sync
await client.resume(doc); // realtime sync
client.changeRealtimeSyncMode(doc, SyncMode.PushOnly);

// provide option
await client.pause(doc); // manual sync
await client.resume(doc, {mode: SyncMode.PushOnly}); // realtime sync

image

What are the relevant tickets?

Fixes #770

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@chacha912 chacha912 marked this pull request as ready for review April 9, 2024 06:11
@chacha912 chacha912 requested a review from hackerwins April 9, 2024 06:11
hackerwins
hackerwins reviewed Apr 9, 2024
View reviewed changes
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

From my understanding, the current structure of the SDK's SyncMode is as follows:

- Realtime: With EventStream
  - PushPull
  - PushOnly
  - SyncOff
- Manual: Without EventStream
  - PushPull
  - PushOnly
  - SyncOff

Even in Manual mode, users can set PushOnly or SyncOff, which may confuse them. How about refactoring SyncMode to have a flat structure for easier understanding?

Proposed Sync Mode options:

- SyncMode.Manual
- SyncMode.Realtime(same with `PushPull`)
- SyncMode.RealtimePushOnly
- SyncMode.RealtimeSyncOff

I think we need to keep Promise in the return and operate asynchronously only when changing from Manual to Realtime.

Perhaps the APIs will also change as shown below:

client.changeSyncMode(doc, SyncMode.Manual); // prev: await client.pause();
client.changeSyncMode(doc, SyncMode.Realtime); // prev: await client.resume();
client.changeSyncMode(doc, SyncMode.RealtimePushOnly); // prev: client.pauseRemoteChanges(doc);
client.changeSyncMode(doc, SyncMode.RealtimeSyncOff); // prev: client.resumeRemoteChanges(doc);

@chacha912
Copy link
Contributor Author

chacha912 commented Apr 11, 2024 •
edited
Loading

@hackerwins Thank you for the suggestion. I agree that refactoring SyncMode into a flat structure is a good idea. So, if I understand correctly, in manual mode, only push-pull synchronization is possible? Previously, users could perform push-only synchronization even in manual mode using the sync(doc, syncMode) function. In that case, I'll also remove the syncMode option from the sync function.

@chacha912
Copy link
Contributor Author

chacha912 commented Apr 11, 2024

@hackerwins I've implemented your suggestions. Additionally, would it be better to change the isRealtimeSync option to syncMode in the attach function as well?

@hackerwins
Copy link
Member

hackerwins commented Apr 11, 2024

@chacha912 That's a good idea. I think it will be more convenient to use.

@chacha912 chacha912 requested a review from hackerwins April 11, 2024 07:05
hackerwins
hackerwins approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

Thank you for handling my requests during the review.
Please update the document too.
https://yorkie.dev/docs/js-sdk

@hackerwins hackerwins changed the title Add SyncOff mode Add RealtimeSyncOff and refactor interface of SyncMode Apr 11, 2024
@hackerwins hackerwins merged commit dc9146f into main Apr 11, 2024
@hackerwins hackerwins deleted the sync-off-mode branch April 11, 2024 07:18
@hackerwins hackerwins mentioned this pull request Apr 11, 2024
Merged
2 tasks
@chacha912 chacha912 mentioned this pull request Apr 11, 2024
Merged
2 tasks
@7hong13 7hong13 mentioned this pull request Apr 15, 2024
Merged
2 tasks
@humdrum humdrum mentioned this pull request Apr 16, 2024
Merged
2 tasks
@hackerwins hackerwins mentioned this pull request Apr 16, 2024
Closed
@hackerwins hackerwins mentioned this pull request Jul 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hackerwins hackerwins hackerwins approved these changes

Assignees

No one assigned

Labels

sdk ⚒️

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide syncoff mode to maintain watch stream without syncing in realtime sync mode

2 participants

@chacha912 @hackerwins